Skip to content

Modernize code related to Surfaces and the SurfaceManager #1498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Modernize code related to Surfaces and the SurfaceManager #1498

wants to merge 7 commits into from

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

  • Modernize code related to Surfaces and the SurfaceManager. Simplify for loops, define default destructors with =default, pass arguments by const reference when possible
  • Make DetectorSurfaces not inherit from DetElement since it is not used nor needed

ENDRELEASENOTES

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2025 •
edited
Loading

Test Results

   18 files     18 suites   7h 13m 17s ⏱️
  372 tests   372 ✅ 0 💤 0 ❌
3 278 runs  3 278 ✅ 0 💤 0 ❌

Results for commit 0a50df9.

♻️ This comment has been updated with latest results.

@andresailer
Copy link
Member

Hi @MarkusFrankATcernch

Could you have a look at these changes? Any reason things were the way they were and shouldn't be changed?

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Oct 3, 2025 •
edited
Loading

@andresailer
Short answer:no idea, Frank should know ;-)

Longer answer:
I do not know why DetectorSurfaces have to be a DetElement. I think this is the main question.
If this is-a-relagtionship is not mandatory, then the changes of Juan are all correct.
If DetectorSurfaces have to be a DetElement then not.
The fact that all DD4hep and ILC tests work is a hint that this inheritance is not necessary.

@gaede : do you remember why this relationship was introduced?

@jmcarcell
Copy link
Member Author

I ran some tracking that makes use of the surfaces before and after this change and I get the same results so I'm relatively confident this is not changing anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarkusFrankATcernch MarkusFrankATcernch Awaiting requested review from MarkusFrankATcernch

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jmcarcell @andresailer @MarkusFrankATcernch